Fix getprotobyname missing on Android preventing use#470
Conversation
getprotobyname is stubbed out on Android causing crash_and_burn to be called. This commit removes the icmp support check when compiling for Android and adds a configuration option to compile without the check. Signed-off-by: Paliak <91493239+Paliak@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configuration option to disable the use of getprotobyname and switches to using hardcoded protocol constants (IPPROTO_ICMP and IPPROTO_ICMPV6) for socket initialization. This change improves compatibility with environments like Android where these lookups may fail. A review comment identifies formatting issues in src/socket6.c, specifically the use of tabs instead of spaces and a minor syntax typo in an if condition.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
|
||
|
|
||
| AC_ARG_ENABLE([getprotobyname], | ||
| AS_HELP_STRING([--disable-getprotobyname], [Disable use of getprotobyname]), |
There was a problem hiding this comment.
Is the --disable-getprotobyname flag really necessary? If not, then an attempt should be made to perform an automatic check.
For example, using AC_CHECK_FUNCS()
There was a problem hiding this comment.
This was the most portable way i could think of implementing this. AC_CHECK_FUNCS will return true on Android as getprotobyname does exist. It just always returns NULL. The only automatic way to check this that i know of would be to use AC_RUN_IFELSE but i decided against that as i feel it would be quite messy (and maybe cause issues with cross compilation). I guess the flag could just be removed altogether as #if !(defined(ANDROID) || defined(__ANDROID__)) would suffice for what this PR is trying to accomplish.
Just thought that it could be a nice to have.
There was a problem hiding this comment.
In that case, it would be better to move the Android detection to configure.ac. We already have the case "${target}" in function, and if other systems use the same approach, it can be extended quite easily.
This keeps the exception in the code itself more flexible.
There was a problem hiding this comment.
I would first check whether getprotobyname is present, and then check whether it needs to be disabled again.
Here's my example:
# Detect Operatingsystem
AC_CANONICAL_TARGET
only_clock_realtime=no
disable_getprotobyname=no
case "${target}" in
*darwin*)
only_clock_realtime=yes
;;
*freebsd*)
only_clock_realtime=yes
;;
*openbsd*)
only_clock_realtime=yes
;;
*android*)
disable_getprotobyname=yes
;;
esac
dnl Checks for libraries.
AC_CHECK_FUNCS([getprotobyname])
AS_IF([test "x$disable_getprotobyname" = "xyes"], [
AC_MSG_NOTICE([Android detected: Disabling getprotobyname])
ac_cv_func_getprotobyname=no
])
if test $ac_cv_func_getprotobyname = yes; then
AC_DEFINE([USE_GETPROTOBYNAME], [1], [Define if getprotobyname is available and should be used.])
fi
There was a problem hiding this comment.
Sorry for the late reply. I've applied your suggestion. Good idea on checking for getprotobyname before disabling it for android specifically. Should help with support for other platforms where the function may be missing, not just android that i'm focused on.
Signed-off-by: Paliak <91493239+Paliak@users.noreply.github.com>
Signed-off-by: Paliak <91493239+Paliak@users.noreply.github.com>
…k/fping into fix-getprotobyname-android
Signed-off-by: Paliak <91493239+Paliak@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces conditional compilation to bypass getprotobyname on platforms like Android, falling back to default protocol constants (IPPROTO_ICMP and IPPROTO_ICMPV6). The reviewer suggests a more robust and idiomatic approach using Autoconf's standard feature detection (AC_CHECK_FUNCS([getprotobyname])) to define HAVE_GETPROTOBYNAME automatically, rather than hardcoding platform-specific checks and using a custom USE_GETPROTOBYNAME variable.
|
coverage: 87.992% (-0.008%) from 88.0% — Paliak:fix-getprotobyname-android into schweikert:develop |
…sabling with android override Change as per schweikert#470 (comment)
3 participants
getprotobynameis stubbed out on Android causing crash_and_burn to be called. This commit removes the ICMP support check when compiling for Android and adds a configuration option to compile withoutgetprotobynamecheck.